[NT-359] Integrate Updating Backing mutation #893

justinswart · 2019-10-11T23:18:32Z

📲 What

A continuation of the work in #887 and #890, this PR is also based off #887's branch.

This wires up the UpdateBacking mutation to allow one to change their payment method and Apple Pay backing.

🤔 Why

This is a continuation of the work to allow a user to manage their pledge.

🛠 How

Added paymentSourceId and ApplePayParams to UpdateBackingData.
Created somewhat duplicate signals to handle the result of the apple pay calls. I think when we do the work for the CreateBacking mutation, I may have a way to make this even more reusable due to that mutation and UpdateBacking being so similar.

👀 See

Note: Currently the manage pledge view does not refresh automatically, this is a known issue that will be addressed in a subsequent PR. Also, currently the credit card that is the current payment source for the backing is meant to be selected by default and ordered first. This will also be addressed in an upcoming PR. So, at the moment, the experience is that when you land on Change payment method, you may see the incorrect card selected, but the behaviour of the submit button enabling/disabling should still be correct and allow you to change it.

✅ Acceptance criteria

Be sure to test this against the native dev environment. The payment method that you use and the project that you back should all be created on that environment. Navigate to Change payment method in the Manage pledge view.

Select a new payment method and hit confirm you should see a success message and your change reflected (with the caveats mentioned above).
Select Apple Pay, the backing should change to an Apple Pay backing*

* I was unable to test this unfortunately as I didn't have an iOS 12 device with me at the time and, although I think it should work, Apple Pay doesn't seem to do anything on the simulator. This is possible a Stripe issue? 🤔

…e Pay

justinswart · 2019-10-11T23:20:41Z

Library/ViewModels/PledgeViewModel.swift

-    // MARK: - Apple Pay
+    // MARK: - Create Apple Pay Backing
+
+    let willCreateApplePayBacking = Signal.combineLatest(


This and willUpdateApplePayBacking are to ensure that upon completion of these asynchronous events we show the appropriate messaging/view to the user.

justinswart · 2019-10-11T23:22:13Z

Library/ViewModels/PledgeViewModel.swift

+
+    // MARK: - Success/Failure Create
+
+    let createPaymentAuthorizationDidFinishSignal = Signal.zip(


zip allows us to ensure that each of these signals will emit at least once before this signal emits. So we know that we started out creating here and not updating because we zip with the willCreateApplePayBacking signal.

…tation

ksr-ci-bot · 2019-10-12T23:15:00Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

ifbarrera

Initial code pass looks good, I'll test this thoroughly tomorrow once we have an Apple Pay sandbox user set up!

Library/ViewModels/PledgeViewModel.swift

ifbarrera · 2019-10-14T18:55:19Z

Library/ViewModels/PledgeViewModel.swift

+    let updateBackingDidCompleteApplePay = updateApplePayBackingSuccess
+      .ignoreValues()
+    let updateBackingDidComplete = Signal.zip(
+      self.submitButtonTappedSignal,


Same here, do we need this zip?

I'm no longer convinced it would do what I need it to here, so I'll consider an alternative.

Library/ViewModels/PledgeViewModelTests.swift

…tation

ifbarrera · 2019-10-16T14:37:40Z

Library/ViewModels/PledgeViewModel.swift


    let createApplePayBackingData = Signal.combineLatest(
      createBackingData,
      pkPaymentData.skipNil(),
      self.stripeTokenSignal.skipNil()
    )
-    .takeWhen(applePayStatusSuccess)
+    .takeWhen(willCreateApplePayBacking.filter(isTrue))


It might make sense to just put this filter(isTrue) as part of willCreateApplePayBacking.

ifbarrera · 2019-10-16T14:38:31Z

Library/ViewModels/PledgeViewModel.swift

+    let updateBackingDataAndIsApplePay = updateBackingData.takePairWhen(
+      Signal.merge(
+        updateButtonTapped.mapConst(false),
+        willUpdateApplePayBacking.filter(isTrue)


Same here would it make sense to put this filter(isTrue) as part of willUpdateApplePayBacking?

ifbarrera · 2019-10-16T14:43:10Z

Library/ViewModels/PledgeViewModel.swift

+      .takeWhen(createApplePayBackingCompleted)
+      .map(first)
+
+    self.confirmationLabelAttributedText = Signal.merge(


Can we move this somewhere higher up in the pledge view model? It's kind of separate from the success/failure stuff.

ifbarrera · 2019-10-16T14:53:34Z

Library/ViewModels/PledgeViewModelTests.swift

@@ -1256,7 +1258,7 @@ final class PledgeViewModelTests: TestCase {
    }
  }

-  func testUpdatingConfirmButtonEnabled_ShippingEnabled() {
+  func testUpdatingSubmitButtonEnabled_ShippingEnabled() {


Did we change the title of the button? 🤔

Yes, it's not always a confirm button anymore, sometimes a pledge button. We should also remove the continue button and just use this one.

ifbarrera · 2019-10-16T14:55:19Z

Library/ViewModels/PledgeViewModelTests.swift

+
+      self.vm.inputs.applePayButtonTapped()
+
+      self.notifyDelegateUpdatePledgeDidSucceedWithMessage.assertDidNotEmitValue()


Can we assert here that the output goToApplePayPaymentAuthorization is called?

ifbarrera · 2019-10-16T14:57:01Z

Library/ViewModels/PledgeViewModelTests.swift

+      self.goToThanks.assertDidNotEmitValue()
+
+      self.vm.inputs.applePayButtonTapped()
+


Same here can we assert that goToApplePayPaymentAuthorization is called?

ifbarrera · 2019-10-16T15:04:44Z

Library/ViewModels/PledgeViewModelTests.swift

+      self.scheduler.run()
+
+      self.notifyDelegateUpdatePledgeDidSucceedWithMessage.assertDidNotEmitValue()
+      self.updatePledgeFailedWithError.assertValues([


Hmm so if the request errors before the payment authorization VC has dismissed, the error banner might show below the payment authorization VC 🤔 that's probably not ideal?

ifbarrera

🎉

* Updated validation * Fix snapshot tests, add submitButtonHidden * Add PledgeAmountSummaryViewController * Add updateButtonTapped * Add snapshot test and vm test * lol * Remove unused var * Rename signal, fix payment source ID comparison * Fix tests * Fix tests, simplify hiding of shipping location stackview * Fix snapshots * Commit the actual snapshots * Use local var * [NT-359] Integrate Updating Backing mutation (#893) * Wire up change payment method to mutation for payment source and Apple Pay * Remove payment method when backing with Apple Pay and vice versa * Update tests for apple pay sheet race condition fixes * Replace zip with simpler signals * Add Stripe token error test * Move signals around, improve tests

Wire up change payment method to mutation for payment source and Appl…

031ebc2

…e Pay

justinswart requested a review from ifbarrera October 11, 2019 23:18

justinswart assigned ifbarrera Oct 11, 2019

justinswart commented Oct 11, 2019

View reviewed changes

justinswart mentioned this pull request Oct 12, 2019

[NT-363] Select a new reward #894

Merged

3 tasks

Merge branch 'change-payment-method-ui' into change-payment-method-mu…

30e77cc

…tation

ifbarrera reviewed Oct 14, 2019

View reviewed changes

justinswart added 6 commits October 15, 2019 12:56

Merge branch 'change-payment-method-ui' into change-payment-method-mu…

189adbe

…tation

Remove payment method when backing with Apple Pay and vice versa

5ae6f86

Update tests for apple pay sheet race condition fixes

d68df53

Replace zip with simpler signals

a8092af

Add Stripe token error test

d65823d

Merge branch 'change-payment-method-ui' into change-payment-method-mu…

6d7bf10

…tation

ifbarrera reviewed Oct 16, 2019

View reviewed changes

Move signals around, improve tests

98c64d6

ifbarrera approved these changes Oct 16, 2019

View reviewed changes

justinswart merged commit e1d63dc into change-payment-method-ui Oct 16, 2019

justinswart deleted the change-payment-method-mutation branch July 14, 2021 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NT-359] Integrate Updating Backing mutation #893

[NT-359] Integrate Updating Backing mutation #893

justinswart commented Oct 11, 2019

justinswart Oct 11, 2019

justinswart Oct 11, 2019

ksr-ci-bot commented Oct 12, 2019

ifbarrera left a comment

ifbarrera Oct 14, 2019

justinswart Oct 16, 2019

ifbarrera Oct 16, 2019

ifbarrera Oct 16, 2019

ifbarrera Oct 16, 2019

ifbarrera Oct 16, 2019

justinswart Oct 16, 2019

ifbarrera Oct 16, 2019

ifbarrera Oct 16, 2019

ifbarrera Oct 16, 2019

ifbarrera left a comment


		// MARK: - Success/Failure Create

		let createPaymentAuthorizationDidFinishSignal = Signal.zip(


		self.vm.inputs.applePayButtonTapped()

		self.notifyDelegateUpdatePledgeDidSucceedWithMessage.assertDidNotEmitValue()

		self.goToThanks.assertDidNotEmitValue()

		self.vm.inputs.applePayButtonTapped()

[NT-359] Integrate Updating Backing mutation #893

[NT-359] Integrate Updating Backing mutation #893

Conversation

justinswart commented Oct 11, 2019

📲 What

🤔 Why

🛠 How

👀 See

✅ Acceptance criteria

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksr-ci-bot commented Oct 12, 2019

ifbarrera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifbarrera left a comment

Choose a reason for hiding this comment